Improve test coverage #12

ydirson · 2022-07-29T13:30:17Z

This brings the test coverage from 38.4% to 66.6%, to help in checking the port to python3.

tests/test_pci.py

stormi · 2022-07-29T14:25:30Z

tests/test_bootloader.py

+            self.assertRegexpMatches(line, r"^(5a6,13$|>)")
+
+
+class TestBootloaderAdHoc(unittest.TestCase):


I wonder if grub1 and extlinux are actually used nowadays.

I too, but the code is still there

Grub1 maybe not. extlinux, yes :-(

stormi · 2022-07-29T14:27:58Z

tests/test_accessor.py

+
+class TestAccessor(unittest.TestCase):
+    def test_http(self):
+        raise unittest.SkipTest("comment out if you really mean it")


Explain this in the commit message?

tests/data/all_ethN.biosdevname

tests/test_cpio.py

stormi · 2022-07-29T15:01:55Z

General comment: a few commit titles are longer than 70 chars (at XCP-ng it's the limit we try not to cross, and that's also when github starts eliding the displayed title). I think you could easily avoid it in most cases by simply prefixing the titles with "Coverage:" rather than "Improve coverage:" (maybe you'll also have to save a few chars here and there too).

ydirson · 2022-08-08T15:42:40Z

General comment: a few commit titles are longer than 70 chars (at XCP-ng it's the limit we try not to cross, and that's also when github starts eliding the displayed title). I think you could easily avoid it in most cases by simply prefixing the titles with "Coverage:" rather than "Improve coverage:" (maybe you'll also have to save a few chars here and there too).

should be OK now

psafont

Really happy to see the tests being improved so much, even catching some bugs. I had a comment but it's not really related to the tests

psafont · 2022-08-09T13:21:52Z

tests/test_cpio.py

+    # CpioFileCompat testing
+
+    def archiveExtractCompat(self, fn, comp):
+        arc = CpioFileCompat(fn, mode="r", compression={"": CPIO_PLAIN,


As an enhancement it'd be nice to get a context manager to close the resource when reaching the end of the scope instead of forcing users to close them every time.

100% agree. This feature has been added to tarfile.py in python3, and we should be able to get it, likely together with other goodies, but we would first have to sync with the 2.7 version, and we don't even have a clue which version was used as a base for this work :/

Python 2.2. Please do feel free to nuke P <2.7-isms.

OK, but that will likely be for a separate PR, let's not mix this with the tests stuff :)

I think we want to stay as close to upstream as possible, and get the <2.7-isms removed from cpython's tarfile first. We'll then get them into cpiofile by merging newer upstream versions.

Adds more data to the archive (a copy of the same file, from another wd). Uses the "." special case for even more coverage. Coverage change: xcp.cpiofile: 50% -> 51.7% total: 38.4% -> 39% Signed-off-by: Yann Dirson <[email protected]>

Coverage change: xcp.cpiofile: 51.7% -> 58.1% total: 39% -> 41.1% Signed-off-by: Yann Dirson <[email protected]>

Coverage change: xcp.pci: 40.9% -> 76.3% total: 41.1% -> 43% Signed-off-by: Yann Dirson <[email protected]>

Coverage change: xcp.cmd: 0% -> 78.0% total: 43% -> 44.2% Signed-off-by: Yann Dirson <[email protected]>

Coverage change: xcp.xmlunwrap: 0% -> 68.6%% total: 44.2% -> 45.2% Signed-off-by: Yann Dirson <[email protected]>

We may want to do some real checks on the output, this first version only exercises the code. Includes ad-hoc testing of write/read methods for grub/extlinux from a grub2 configuration, for lack of representative versions of those deprecated config files. Ad-hoc test for extlinux reveals inconsistent typing, and is marked as failing for now: print("serial %d %d" % (self.serial['port'], > self.serial['baud']), file=fh) E TypeError: %d format: a number is required, not str Coverage change: xcp.bootloader: 0% -> 59.1%% total: 45.2% -> 53.4% Signed-off-by: Yann Dirson <[email protected]>

File format parsers do not all agree whether to store some parameters as int or str. This relax the format strings to accept both. Fixes creating an extlinux config from a grub2 config. Coverage change: xcp.bootloader: 59.1% -> 72.8% total: 53.4% -> 55.3% Signed-off-by: Yann Dirson <[email protected]>

Only FileAccessor is actually covered by these tests. HTTPAccessor tests are provided, but are skipped by default, as they currently need network access to xcp-ng.org repository. We will want at some point to mock the HTTP access. Coverage change: xcp.accessor: 0% -> 30.0% xcp.repository: 0% -> 72.2% total: 55.3% -> 62.7% Signed-off-by: Yann Dirson <[email protected]>

Coverage change: xcp.environ: 0% -> 80.0% total: 62.7% -> 63.3% Signed-off-by: Yann Dirson <[email protected]>

Direct test execution only brings us dependency on `sys` :) Signed-off-by: Yann Dirson <[email protected]>

Coverage change: xcp.net.biosdevname: 41.5% to 87.8% total: 63.3% -> 63.8% Signed-off-by: Yann Dirson <[email protected]>

This function is private, not called, and if called would not do what the docstring says (does not pass "eth" as argument to the command called). Coverage change: xcp.net.biosdevname: 87.8% -> 94.6% total: 63.8% -> 63.9% Signed-off-by: Yann Dirson <[email protected]>

Coverage change: xcp.cpiofile: 58.1% -> 62.1% total: 63.9% -> 65.2% Signed-off-by: Yann Dirson <[email protected]>

Coverage change: xcp.bootloader: 72.8% -> 81.9% total: 65.4% -> 66.6% Signed-off-by: Yann Dirson <[email protected]>

Signed-off-by: Yann Dirson <[email protected]>

edwintorok

This changes/introduces test code, except for:

removal of some main from certain files and moving it into test code (ok)
changing some formats from %d to %s, apparently this fixes a bug in a unit test that previously was marked as FIXME, so this should be fine

…5506-improve-tests CP-45506: Verify that /etc/systemd does not have files(moved to tarball)

stormi reviewed Jul 29, 2022

View reviewed changes

ydirson force-pushed the improve-test-coverage branch 2 times, most recently from c1e4eba to daeca57 Compare July 29, 2022 15:38

ydirson force-pushed the improve-test-coverage branch from daeca57 to e1474df Compare August 8, 2022 15:15

psafont self-requested a review August 9, 2022 08:27

ydirson force-pushed the improve-test-coverage branch 2 times, most recently from d4acc74 to e314350 Compare August 9, 2022 12:31

psafont approved these changes Aug 9, 2022

View reviewed changes

ydirson force-pushed the improve-test-coverage branch from e314350 to af3e5a0 Compare August 9, 2022 13:47

ydirson added 7 commits August 29, 2022 18:00

Coverage: test_cpio: recursive add to archive

92c4403

Adds more data to the archive (a copy of the same file, from another wd). Uses the "." special case for even more coverage. Coverage change: xcp.cpiofile: 50% -> 51.7% total: 38.4% -> 39% Signed-off-by: Yann Dirson <[email protected]>

Coverage: test_cpio: exercise "extractall()" and check its output

925e330

Coverage change: xcp.cpiofile: 51.7% -> 58.1% total: 39% -> 41.1% Signed-off-by: Yann Dirson <[email protected]>

Coverage: test_pci: new test from snippet in pci.py

d8fdc3d

Coverage change: xcp.pci: 40.9% -> 76.3% total: 41.1% -> 43% Signed-off-by: Yann Dirson <[email protected]>

Coverage: new test_cmd from snippet in cmd.py

02468d4

Coverage change: xcp.cmd: 0% -> 78.0% total: 43% -> 44.2% Signed-off-by: Yann Dirson <[email protected]>

Coverage: new test_xmlunwrap from snippet in xmlunwrap.py

b1a5152

Coverage change: xcp.xmlunwrap: 0% -> 68.6%% total: 44.2% -> 45.2% Signed-off-by: Yann Dirson <[email protected]>

ydirson force-pushed the improve-test-coverage branch 2 times, most recently from 89011f4 to d38a648 Compare August 30, 2022 14:02

ydirson added 8 commits September 26, 2022 13:41

coverage: new test_environ: parse a sample "xensource-inventory"

c25da36

Coverage change: xcp.environ: 0% -> 80.0% total: 62.7% -> 63.3% Signed-off-by: Yann Dirson <[email protected]>

tests: remove unused imports, direct test execution

c364a31

Direct test execution only brings us dependency on `sys` :) Signed-off-by: Yann Dirson <[email protected]>

Coverage: test_biosdevname: add a test of all_devices_all_names()

57a35b0

Coverage change: xcp.net.biosdevname: 41.5% to 87.8% total: 63.3% -> 63.8% Signed-off-by: Yann Dirson <[email protected]>

Coverage: test_cpio: add a test for CpioFileCompat

935f932

Coverage change: xcp.cpiofile: 58.1% -> 62.1% total: 63.9% -> 65.2% Signed-off-by: Yann Dirson <[email protected]>

Coverage: add a test for Bootloader.newDefault

8244c49

Coverage change: xcp.bootloader: 72.8% -> 81.9% total: 65.4% -> 66.6% Signed-off-by: Yann Dirson <[email protected]>

cpiofile: flag known issue in apparently-unused code

056238e

Signed-off-by: Yann Dirson <[email protected]>

ydirson force-pushed the improve-test-coverage branch from d38a648 to 056238e Compare September 26, 2022 11:48

ydirson mentioned this pull request Sep 26, 2022

Python3 compatibility #5

Closed

edwintorok approved these changes Sep 26, 2022

View reviewed changes

edwintorok merged commit decbb50 into xenserver:master Sep 26, 2022

ydirson deleted the improve-test-coverage branch September 29, 2022 13:21

bernhardkaindl added a commit to rosslagerwall/python-libs that referenced this pull request May 8, 2024

Merge pull request xenserver#12 from xenserver/private/bernhardk/CP-4…

5013b05

…5506-improve-tests CP-45506: Verify that /etc/systemd does not have files(moved to tarball)

		self.assertRegexpMatches(line, r"^(5a6,13$\|>)")


		class TestBootloaderAdHoc(unittest.TestCase):

Improve test coverage #12

Improve test coverage #12

Uh oh!

Conversation

ydirson commented Jul 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

stormi commented Jul 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ydirson commented Aug 8, 2022

Uh oh!

psafont left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andyhhp Aug 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edwintorok left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ydirson commented Jul 29, 2022 •

edited

Loading

stormi commented Jul 29, 2022 •

edited

Loading

andyhhp Aug 9, 2022 •

edited

Loading